Skip to content

Render strings in bedrock test cassettes for readability #3243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 6, 2025
Merged

Render strings in bedrock test cassettes for readability #3243

merged 2 commits into from
Feb 6, 2025

Conversation

anuraaga
Copy link
Contributor

@anuraaga anuraaga commented Feb 6, 2025 •
edited
Loading

Description

Previously API recordings for AWS SDK Bedrock instrumentation tests were formatted as binary making them unreadable in things like code review. This formats them to

  • Pretty-print raw JSON in request and response where possible (bedrock streaming responses are actually a binary protocol and remain as binary)
  • Print request headers as strings
  • Use multiline strings for long ones

/cc @xrmx @codefromthecrypt

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Unit tests

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

anuraaga
anuraaga commented Feb 6, 2025
View reviewed changes
@codefromthecrypt codefromthecrypt added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 6, 2025
codefromthecrypt
codefromthecrypt approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@codefromthecrypt codefromthecrypt left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better as at least the request side is completely readable now, as is the response side non-streaming. Streaming responses use a binary format, so while the body cannot be improved, the headers are.

Good work!

xrmx
xrmx approved these changes Feb 6, 2025
View reviewed changes
Copy link
Contributor

@xrmx xrmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@xrmx xrmx merged commit 6b3a11b into open-telemetry:main Feb 6, 2025
694 of 695 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@codefromthecrypt codefromthecrypt codefromthecrypt approved these changes

@xrmx xrmx xrmx approved these changes

Assignees

No one assigned

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anuraaga @xrmx @codefromthecrypt